solarcam: run ffmpeg without a shell; camera-supplied file names reached it by openipc-ai · Pull Request #15 · OpenIPC/python-dvr

openipc-ai · 2026-10-27T06:46:25Z

SolarCam.convertFile() ran ffmpeg through shell=True. The paths it was given include parts of the FileName the camera returns, so a hostile or compromised camera could run commands on the machine running download-local-files.py (the Docker image's CMD). This only happens when target_filetype_video is set. Reported by @waseem-cve (filed as GHSA-w2h5-ww59-7hxr on OpenIPC/firmware, where this code does not live).

Changes (`solarcam.py` only)

convertFile() passes ffmpeg an argv list instead of a shell string. The flags are unchanged.
generateTargetFilePath() keeps only [A-Za-z0-9_-] in the name it builds from the camera's segments. This is defence in depth, and it also keeps shell and filesystem oddities out of the download dir. Real names come out unchanged (2024-01-15_12-00-00), so existing blacklists and .dmp dumps still match.
convertFile() used to log "successfully converted" and delete the downloaded recording even when ffmpeg failed. It now returns early on failure and keeps the source file.

Testing

I used a harness that stubs DVRIPCam.download_file and puts a fake ffmpeg on PATH. The fake records its argv and exits with 0 or 1. Each run calls save_files(dir, [file], target_filetype=".webm") with one benign and one malicious FileName:

benign: /mnt/sd/2024-01-15/001/12.00.00-12.05.00[R][@1a][0].h264
malicious: /mnt/sd/x;touch${IFS}PWNED;/001/12.00.00-12.05.00[R][@1a][0].h264

Before (master 6ea861a):

benign:
  rc=0 derived=dl/2024-01-15_12-00-00.h264
  PWNED marker: False
  dl/: []
  ffmpeg argv: [-framerate][15][-i][/tmp/W/dl/2024-01-15_12-00-00.h264][-b:v][1M][-c:v][libvpx-vp9][-c:a][libopus][/tmp/W/dl/2024-01-15_12-00-00.webm]
benign:
  rc=1 derived=dl/2024-01-15_12-00-00.h264
  PWNED marker: False
  dl/: []
  ffmpeg argv: [-framerate][15][-i][/tmp/W/dl/2024-01-15_12-00-00.h264][-b:v][1M][-c:v][libvpx-vp9][-c:a][libopus][/tmp/W/dl/2024-01-15_12-00-00.webm]
malicious:
  rc=0 derived=dl/x;touch${IFS}PWNED;_12-00-00.h264
  PWNED marker: True
  dl/: []
  ffmpeg argv: [-framerate][15][-i][/tmp/W/dl/x]
malicious:
  rc=1 derived=dl/x;touch${IFS}PWNED;_12-00-00.h264
  PWNED marker: True
  dl/: []
  ffmpeg argv: [-framerate][15][-i][/tmp/W/dl/x]

After:

benign:
  rc=0 derived=dl/2024-01-15_12-00-00.h264
  PWNED marker: False
  dl/: []
  ffmpeg argv: [-framerate][15][-i][/tmp/W/dl/2024-01-15_12-00-00.h264][-b:v][1M][-c:v][libvpx-vp9][-c:a][libopus][/tmp/W/dl/2024-01-15_12-00-00.webm]
benign:
  rc=1 derived=dl/2024-01-15_12-00-00.h264
  PWNED marker: False
  dl/: ['2024-01-15_12-00-00.h264']
  ffmpeg argv: [-framerate][15][-i][/tmp/W/dl/2024-01-15_12-00-00.h264][-b:v][1M][-c:v][libvpx-vp9][-c:a][libopus][/tmp/W/dl/2024-01-15_12-00-00.webm]
malicious:
  rc=0 derived=dl/x_touch__IFS_PWNED__12-00-00.h264
  PWNED marker: False
  dl/: []
  ffmpeg argv: [-framerate][15][-i][/tmp/W/dl/x_touch__IFS_PWNED__12-00-00.h264][-b:v][1M][-c:v][libvpx-vp9][-c:a][libopus][/tmp/W/dl/x_touch__IFS_PWNED__12-00-00.webm]
malicious:
  rc=1 derived=dl/x_touch__IFS_PWNED__12-00-00.h264
  PWNED marker: False
  dl/: ['x_touch__IFS_PWNED__12-00-00.h264']
  ffmpeg argv: [-framerate][15][-i][/tmp/W/dl/x_touch__IFS_PWNED__12-00-00.h264][-b:v][1M][-c:v][libvpx-vp9][-c:a][libopus][/tmp/W/dl/x_touch__IFS_PWNED__12-00-00.webm]

Before the fix, the malicious name creates PWNED and ffmpeg only receives .../dl/x. On rc=1 the benign recording is deleted (dl/: []). After the fix there's no marker, the malicious name is flattened to x_touch__IFS_PWNED__12-00-00, the benign path is byte-identical, and on rc=1 the source is kept. (dl/ is empty on rc=0 because the fake ffmpeg writes no output and the source is then removed, as intended.)

python3 -m py_compile solarcam.py passes. Not run against a real solar camera or a real ffmpeg.

qodo-free-for-open-source-projects · 2026-10-27T06:46:56Z

PR Summary by Qodo

Prevent shell injection in SolarCam video conversion

🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Run ffmpeg without a shell so camera-supplied filenames cannot execute host commands.
• Sanitize derived download names while preserving the format of normal recordings.
• Keep downloaded recordings when conversion fails instead of reporting success and deleting them.

Diagram

graph TD
  Camera["Camera files"] --> Paths["Sanitized paths"] --> Download["Download recording"] --> Convert{"Conversion enabled?"}
  Convert -->|Yes| FFmpeg["ffmpeg argv"] --> Result{"Conversion succeeds?"} -->|No| Keep["Retain source"]
  Result -->|Yes| Delete["Remove source"]
  Convert -->|No| Keep

High-Level Assessment

The combined approach is appropriate: passing argv removes shell interpretation, while filename sanitization protects local paths. Sanitization alone would leave the shell boundary in place. The submitted harness exercises benign and malicious names and both ffmpeg exit statuses, but no automated test is included in the diff.

Files changed (1) +14 / -2

Bug fix (1) +14 / -2

solarcam.pySecure derived filenames and ffmpeg conversion +14/-2

Secure derived filenames and ffmpeg conversion

• Replaces characters outside [A-Za-z0-9_-] in camera-derived recording names. Invokes ffmpeg with an argument list instead of shell=True, preserving its flags. Returns on a nonzero ffmpeg exit status so the downloaded source is not logged as converted or deleted.

solarcam.py

qodo-free-for-open-source-projects · 2026-10-27T06:48:17Z

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. ~~Failed conversions never retry~~ ✓ Resolved

Description

convertFile() now retains the source on a nonzero ffmpeg exit, but save_files() treats any
existing source as complete before it considers conversion. On every later polling cycle, it skips
that recording, so a transient conversion failure cannot recover without manual intervention.

Code

solarcam.py[105]

+            return

Evidence
A nonzero exit now returns without unlinking the source; the source-existence check then precedes
the converted-output check and continues past conversion on subsequent calls.
solarcam.py[102-108]
solarcam.py[125-135]
solarcam.py[152-157]
download-local-files.py[89-97]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A failed conversion leaves its source in place, but the next polling cycle skips the source rather than retrying conversion.
## Fix Focus Areas
- solarcam.py[102-105]
- solarcam.py[125-135]
## Recommended Fix
When a source exists and its converted output is absent, retry conversion from that source without downloading it again. Handle any partial converted output before retrying, and preserve the source until conversion succeeds.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. Distinct recordings can be skipped 🐞 Bug ≡ Correctness

Description

generateTargetFilePath() replaces every disallowed character with _, so camera names containing
: and _ can produce the same local basename. If one recording reaches that path first,
save_files() sees an existing file or matching blacklist entry and skips the other recording
without downloading it.

Code

solarcam.py[78]

+        filenameDisk = re.sub(r"[^A-Za-z0-9_-]", "_", filenameDisk)

Evidence

The newly added substitution maps distinct selected components such as CAM:d and CAM_d to the
same basename. Download existence and blacklist checks use only that generated path to decide
whether to skip a camera file.

solarcam.py[73-84]
solarcam.py[115-150]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Lossy character replacement can map distinct camera filenames onto one local path, which is used to decide whether a recording has already been handled.
## Fix Focus Areas
- solarcam.py[74-82]
- solarcam.py[115-145]
## Recommended Fix
Use a deterministic filename encoding or identity suffix that distinguishes camera filenames containing replaced characters from names already containing underscores. Apply the same mapping to download paths and blacklist entries.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. Older blacklist entries stop matching 🐞 Bug ≡ Correctness

Description

generateTargetFilePath() now changes local paths for camera names with characters other than
letters, digits, underscores, hyphens, and periods, but save_files() compares only the new paths.
When a deployment retains an existing download or blacklist entry for such a name, the same camera
recording is no longer recognized and can be downloaded again.

Code

solarcam.py[78]

+        filenameDisk = re.sub(r"[^A-Za-z0-9_-]", "_", filenameDisk)

Evidence

The old basename construction replaced periods only; the new substitution changes other characters.
Both persisted dump entries and the current existence and blacklist checks are derived from the
generated path, with no lookup for its earlier form.

solarcam.py[60-70]
solarcam.py[73-84]
solarcam.py[125-145]
download-local-files.py[55-58]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Existing downloads and blacklist entries made from nonstandard camera names use the old path, while subsequent checks use only the sanitized path.
## Fix Focus Areas
- solarcam.py[74-82]
- solarcam.py[125-145]
- solarcam.py[60-70]
## Recommended Fix
Provide a migration or compatibility lookup for legacy paths and blacklist entries before treating a camera recording as new. Continue using safe paths for any new downloads.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Tip of the day

💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

@waseem-cve

…hed it convertFile() built the ffmpeg command as one f-string and ran it with shell=True. Its paths come from generateTargetFilePath(), which takes segments of the FileName the camera returns from OPFileQuery and only replaces "." with "-". A camera returning /mnt/sd/x;touch${IFS}PWNED;/001/... therefore ran commands on the downloading host whenever target_filetype_video was set. ffmpeg now gets an argv list, and generateTargetFilePath() keeps only [A-Za-z0-9_-] in the name it derives. Real names (2024-01-15_12-00-00) are unchanged, so existing blacklists and .dmp entries still match. convertFile() also logged success and deleted the downloaded recording even when ffmpeg failed. It now keeps the source on failure. Reported by @waseem-cve.

Keeping the source after a failed ffmpeg run left save_files() treating it as done, so the conversion was never retried. With conversion on, an existing source is now converted again without a second download, and a partial output from the failed run is removed so it is not mistaken for a finished conversion.

qodo-free-for-open-source-projects Bot reviewed Oct 27, 2026

View reviewed changes

Comment thread solarcam.py Resolved

Comment thread solarcam.py Resolved

Comment thread solarcam.py Resolved

openipc-ai added 2 commits October 27, 2026 07:07

openipc-ai force-pushed the solarcam-no-shell branch from a26acb0 to 62c8e47 Compare October 27, 2026 07:07

openipc-ai merged commit dfa77ef into master Oct 27, 2026
6 checks passed

openipc-ai deleted the solarcam-no-shell branch October 27, 2026 07:12

Navigation Menu

Repository navigation

solarcam: run ffmpeg without a shell; camera-supplied file names reached it - #15

solarcam: run ffmpeg without a shell; camera-supplied file names reached it#15
openipc-ai merged 2 commits into
masterfrom
solarcam-no-shell

openipc-ai commented Oct 27, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Oct 27, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Oct 27, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

openipc-ai commented Oct 27, 2026

Changes (solarcam.py only)

Testing

Uh oh!

qodo-free-for-open-source-projects Bot commented Oct 27, 2026

PR Summary by Qodo

Uh oh!

qodo-free-for-open-source-projects Bot commented Oct 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Changes (`solarcam.py` only)

qodo-free-for-open-source-projects Bot commented Oct 27, 2026 •

edited

Loading